Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose utility for logging video frames for an entire video #7421

Merged
merged 22 commits into from
Sep 17, 2024
Merged

Expose utility for logging video frames for an entire video #7421

merged 22 commits into from
Sep 17, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Sep 16, 2024 •
edited
Loading

What

Had to patch this through to all 3 sdks which naturally takes quite a bit of extra machinery.
Decided to go with raw ns here since it's easiest to pass through FFI and is the most versatile (hopefully our VideoFrameReference typing will be solved with tags instead of per component enums in the future, making this more equivalent).

Changed snippets around. There's now:

  • a sample with two single frozen frames, showing next to each other. Demonstrating that video frames can be used individually and without any extra utilities
  • sample using the new frame extraction utility, essentially a "send full video" which we may encapsulate into a higher level utility at some point. It's quite short though in Python, so I'm not too worried about this!

Asset drag & drop got updated to also use this utility.
All this churn also led me to changing how re_video is used a bit, making the entry point more highlevel. Media type got a bit annoying there because we can't afford re_types dependencies in the C++ & Rust SDKs. The solution here is to have independent media type parsing for videos in re_video.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide
  • pass main ci

To run all checks from main, comment on the PR with @rerun-bot full-check.

Wumpf and others added 12 commits September 16, 2024 12:15
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 16, 2024 •
edited
Loading

Deployed docs

Commit Link
bb15253 https://landing-mz98v4to0-rerun.vercel.app/docs

@Wumpf Wumpf added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Sep 16, 2024
@jleibs jleibs self-requested a review September 16, 2024 13:38
@Wumpf Wumpf force-pushed the andreas/video-timestamp-util branch from d8922f9 to 5049e93 Compare September 16, 2024 14:28
@Wumpf Wumpf requested a review from jleibs September 16, 2024 14:28
Wumpf
Wumpf commented Sep 16, 2024
View reviewed changes
rerun_cpp/src/rerun/result.hpp Outdated Show resolved Hide resolved
jleibs
jleibs approved these changes Sep 16, 2024
View reviewed changes
crates/store/re_video/src/lib.rs Show resolved Hide resolved
crates/store/re_video/src/mp4.rs
/// Returns whether a buffer is MP4 video data.
///
/// From `infer` crate.
pub fn is_mp4(buf: &[u8]) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, not error prone at all 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to take suggestions ;-)

crates/top/rerun_c/src/video.rs
extern "C" fn(context: *mut std::ffi::c_void, num_timestamps: u32) -> *mut i64,
>,
error: *mut CError,
) -> *mut i64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bare pointer in the signature seems kind of awkward and hard-to-use without knowledge of the num_timestamps param that was passed into the alloc_func.

Do we need this for anything / does it make some common pattern more ergonomic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed it orginally but didn't bother to remove it. Let's clean this up!

crates/top/rerun_c/src/video.rs

#[allow(unsafe_code)]
#[no_mangle]
pub extern "C" fn rr_video_asset_read_frame_timestamps_ns(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More work than you likely want to tackle today, but once we have "dataframe" APIs in rerun_c, I think this is a good candidate to return the same arrow-based columnar structures we use in our query APIs instead of a bare array like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed on both accounts 😄

crates/viewer/re_context_menu/src/actions/add_entities_to_new_space_view.rs Outdated Show resolved Hide resolved
rerun_cpp/src/rerun/error.hpp Show resolved Hide resolved
rerun_cpp/src/rerun/result.hpp Outdated Show resolved Hide resolved
rerun_py/rerun_sdk/rerun/archetypes/asset_video_ext.py Show resolved Hide resolved
@Wumpf
Copy link
Member Author

Wumpf commented Sep 17, 2024

@rerun-bot full-check

@github-actions GitHub Actions
Copy link

Started a full build: https://github.com/rerun-io/rerun/actions/runs/10900732294

@Wumpf Wumpf merged commit 9fe16ba into main Sep 17, 2024
71 of 73 checks passed
@Wumpf Wumpf deleted the andreas/video-timestamp-util branch September 17, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleibs jleibs jleibs approved these changes

Assignees
No one assigned
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🎞️ video
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Video frame references API / dataloader for logging a video file directly
2 participants
@Wumpf @jleibs